refactor(tests): CMake-based CDB, workspace fixture, test cleanup - #378

tests/conftest.py (1)

199-210: Consider parameterizing language_id for broader file type support.

The helper hardcodes language_id="cpp" for all files. While this works for .cpp files, the module tests in test_modules.py open .cppm files through this helper. If the server relies on language_id for module-specific processing, this could cause issues.

Consider allowing callers to override the language ID when needed:

♻️ Optional: Accept language_id parameter

-def lsp_open(client, filepath: Path, version: int = 0):
+def lsp_open(client, filepath: Path, version: int = 0, language_id: str = "cpp"):
     """Open a text document and return (uri, content)."""
     content = filepath.read_text(encoding="utf-8")
     uri = filepath.as_uri()
     client.text_document_did_open(
         DidOpenTextDocumentParams(
             text_document=TextDocumentItem(
-                uri=uri, language_id="cpp", version=version, text=content
+                uri=uri, language_id=language_id, version=version, text=content
             )
         )
     )
     return uri, content

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 199 - 210, The helper lsp_open currently
hardcodes language_id="cpp"; change its signature to accept an optional
language_id parameter (default "cpp") and pass that value into TextDocumentItem
when calling client.text_document_did_open so callers can override for .cppm or
other files; update any tests that call lsp_open to pass the appropriate
language_id where needed (references: lsp_open function and TextDocumentItem in
tests/conftest.py).

tests/unit/feature/code_completion_tests.cpp (1)

17-31: Prefer function-local vfs/main_path to avoid shared mutable test state.

Keeping these at suite scope is unnecessary and can make parallel runs flaky. Localize both inside code_complete().

Refactor sketch

-llvm::IntrusiveRefCntPtr<TestVFS> vfs;
-std::string main_path;
@@
 void code_complete(llvm::StringRef code) {
-    vfs = llvm::makeIntrusiveRefCnt<TestVFS>();
+    auto vfs = llvm::makeIntrusiveRefCnt<TestVFS>();
@@
-    main_path = TestVFS::path("main.cpp");
+    auto main_path = TestVFS::path("main.cpp");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/feature/code_completion_tests.cpp` around lines 17 - 31, The
global mutable variables vfs and main_path should be made local to
code_complete() to avoid shared test state; remove the suite-scope declarations
of llvm::IntrusiveRefCntPtr<TestVFS> vfs and std::string main_path and instead
create local variables inside code_complete() (e.g.,
llvm::IntrusiveRefCntPtr<TestVFS> vfs = llvm::makeIntrusiveRefCnt<TestVFS>();
and std::string main_path = TestVFS::path("main.cpp")), update uses in the
function where CompilationParams params, AnnotatedSource::from(code),
vfs->add(...), params.vfs = vfs, params.arguments and params.completion
reference them, and ensure no other tests rely on the removed globals.

tests/unit/compile/compilation_tests.cpp (2)

242-273: Test does not verify PCH reuse with the second content version.

The test computes bounds for both content_v1 and content_v2 and verifies they're equal, but only compiles v1. To fully test that "modifying code after the preamble should not require PCH rebuild," the test should also compile v2 using the existing PCH and verify it succeeds.

♻️ Suggested enhancement to complete the test

     // Build PCH with v1.
     add_main("main.cpp", content_v1);
     ASSERT_TRUE(compile_with_pch());
     ASSERT_TRUE(unit.has_value());
     ASSERT_TRUE(unit->top_level_decls().size() >= 1U);
+
+    // Verify v2 can reuse the same PCH (bound is identical).
+    // Note: This would require exposing the PCH or modifying compile_with_pch
+    // to accept pre-built PCH info, which may be out of scope for this PR.
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/compile/compilation_tests.cpp` around lines 242 - 273, The test
PCHContentDifference computes preamble bounds for content_v1 and content_v2 but
never verifies PCH reuse for content_v2; after building the PCH with content_v1
(using add_main("main.cpp", content_v1) and ASSERT_TRUE(compile_with_pch())),
update the test to replace the main with content_v2 (e.g., call
add_main("main.cpp", content_v2)), invoke compile_with_pch() again and
ASSERT_TRUE on its result, then assert unit.has_value() and that
unit->top_level_decls().size() is >= 1U to confirm compilation succeeded without
rebuilding PCH.

---

175-240: Test cleanup may leave orphaned files on early assertion failure.

The temp PCM files created via fs::createTemporaryFile are only cleaned up at the end of the test. If any ASSERT_* fails before lines 238-239, the files will remain. Consider using RAII-based cleanup or moving cleanup to a scope guard.

♻️ Optional: RAII-based cleanup pattern

+    // Helper for automatic cleanup
+    auto cleanup_file = [](const std::string& path) {
+        llvm::sys::fs::remove(path);
+    };
+    std::unique_ptr<std::string, decltype(cleanup_file)> pcm_b_guard;
+    std::unique_ptr<std::string, decltype(cleanup_file)> pcm_a_guard;
+
     auto pcm_b_path = fs::createTemporaryFile("mod_b", "pcm");
     ASSERT_TRUE(pcm_b_path.operator bool());
+    pcm_b_guard = std::unique_ptr<std::string, decltype(cleanup_file)>(
+        new std::string(*pcm_b_path), cleanup_file);
     params_b.output_file = *pcm_b_path;
     // ... similar for pcm_a_path ...
-
-    // Clean up temp PCM files.
-    llvm::sys::fs::remove(*pcm_b_path);
-    llvm::sys::fs::remove(*pcm_a_path);

Alternatively, if the test framework supports it, use a test fixture with teardown.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/compile/compilation_tests.cpp` around lines 175 - 240, The test
currently creates temporary PCM files via fs::createTemporaryFile (pcm_b_path,
pcm_a_path) and only calls llvm::sys::fs::remove at the end, which leaks files
on early ASSERT_* failures; wrap the temporary-paths in an RAII cleanup (e.g., a
small ScopedTempFile or scope guard) that stores the Optional<Path> returned by
fs::createTemporaryFile and removes the file in its destructor, or register a
teardown/scope-exit that always calls llvm::sys::fs::remove for pcm_b_path and
pcm_a_path so cleanup runs even if unit_b or unit_a assertions fail.

tests/unit/test/tester.cpp (1)

201-281: Consider extracting common setup logic to reduce duplication.

The initial setup (lines 202-217) duplicates prepare_driver()'s argument resolution logic. Similarly, the file remapping loops (lines 235-243 and 262-269) share identical path-handling logic. While the current implementation is correct, extracting a helper could improve maintainability.

♻️ Optional: Extract shared logic

// Private helper for driver argument setup
void Tester::setup_driver_args(llvm::StringRef standard) {
    params = CompilationParams();
    unit.reset();
    vfs = llvm::makeIntrusiveRefCnt<TestVFS>();
    for(auto& [file, source]: sources.all_files) {
        vfs->add(file, source.content);
    }

    auto command = std::format("clang++ {} {} -fms-extensions", standard, src_path);
    database.add_command("fake", src_path, command);

    CommandOptions options;
    options.query_toolchain = true;
    options.suppress_logging = true;
    auto commands = database.lookup(src_path, options);
    assert(!commands.empty() && "lookup failed after add_command");
    params.arguments = commands.front().arguments;
}

// Private helper for remapping files
void Tester::remap_sources(std::optional<std::size_t> main_bound = std::nullopt) {
    for(auto& [file, source]: sources.all_files) {
        if(file == src_path) {
            if(main_bound) {
                params.add_remapped_file(file, source.content, *main_bound);
            } else {
                params.add_remapped_file(file, source.content);
            }
        } else {
            std::string path = path::is_absolute(file) ? file.str() : path::join(".", file);
            params.add_remapped_file(path, source.content);
        }
    }
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test/tester.cpp` around lines 201 - 281, The method
Tester::compile_driver_with_pch duplicates driver argument resolution and
file-remapping logic; extract the initial setup block (params reset, unit.reset,
vfs creation and population, command creation/database.add_command,
CommandOptions lookup and params.arguments assignment) into a new private helper
Tester::setup_driver_args(llvm::StringRef standard) and replace the duplicated
block with a call to it, and extract the two remapping loops into a private
Tester::remap_sources(std::optional<std::size_t> main_bound = std::nullopt) that
adds remapped files (using path::is_absolute/file.str or path::join for
non-src_path entries and the optional bound for the main file); then call
remap_sources(bound) for the preamble phase and remap_sources() for the content
phase, and update any other callers like prepare_driver() to use
setup_driver_args.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/unit/test/cdb_helper.h (1)

62-65: Expose load() result to avoid silent test setup failures.

On Line 64, the loaded-entry count is discarded. Returning it from write_cdb(...) makes failures easier to catch at call sites (especially when JSON is malformed or file creation fails).

Suggested diff

-inline void write_cdb(TempDir& tmp, CompilationDatabase& cdb, llvm::StringRef json_content) {
+inline std::size_t write_cdb(TempDir& tmp, CompilationDatabase& cdb, llvm::StringRef json_content) {
     tmp.touch("compile_commands.json", json_content);
-    cdb.load(tmp.path("compile_commands.json"));
+    return cdb.load(tmp.path("compile_commands.json"));
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test/cdb_helper.h` around lines 62 - 65, The helper write_cdb
currently discards the return value of CompilationDatabase::load which can hide
test setup failures; change write_cdb(TempDir& tmp, CompilationDatabase& cdb,
llvm::StringRef json_content) to return the load result (e.g., an int or size_t)
and propagate the value from cdb.load(tmp.path("compile_commands.json")) so
callers can assert the loaded-entry count after creating the
compile_commands.json file via TempDir::touch; ensure callers in tests are
updated to check the returned count where appropriate.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/data/modules/independent_modules/CMakeLists.txt (1)

4-5: Make the C++ standard non-negotiable in this fixture.

Please add CMAKE_CXX_STANDARD_REQUIRED ON to avoid silent downgrade from C++20.

Suggested patch

 set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/data/modules/independent_modules/CMakeLists.txt` around lines 4 - 5,
Add an explicit requirement that the C++ standard cannot be downgraded by
setting CMAKE_CXX_STANDARD_REQUIRED ON in the CMakeLists fixture: locate the
existing set(CMAKE_CXX_STANDARD 20) (and the related set(CMAKE_CXX_EXTENSIONS
OFF)) and insert a set(CMAKE_CXX_STANDARD_REQUIRED ON) immediately after them so
CMake enforces C++20.

tests/data/modules/deep_chain/CMakeLists.txt (1)

4-5: Require C++20 explicitly to keep fixture behavior deterministic.

Add CMAKE_CXX_STANDARD_REQUIRED ON so CI/toolchain differences can’t downgrade the language mode.

Suggested patch

 set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/data/modules/deep_chain/CMakeLists.txt` around lines 4 - 5, Add an
explicit requirement that the C++ standard not be downgraded by setting
CMAKE_CXX_STANDARD_REQUIRED ON in the same CMakeLists context that defines the
standard (near set(CMAKE_CXX_STANDARD 20) and set(CMAKE_CXX_EXTENSIONS OFF));
this ensures the C++20 mode is enforced across CI/toolchains by making
CMAKE_CXX_STANDARD required rather than optional.

tests/data/modules/module_partitions/CMakeLists.txt (1)

4-5: Enforce C++20 as a hard requirement for this fixture.

Set CMAKE_CXX_STANDARD_REQUIRED ON so this test fixture never silently falls back to an older standard on unsupported toolchains.

Suggested patch

 set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/data/modules/module_partitions/CMakeLists.txt` around lines 4 - 5, The
CMakeLists currently sets CMAKE_CXX_STANDARD to 20 but doesn't enforce it;
update the fixture to require C++20 by adding a directive to enable strict
standard enforcement—specifically add CMAKE_CXX_STANDARD_REQUIRED set to ON (in
the same scope where set(CMAKE_CXX_STANDARD 20) and set(CMAKE_CXX_EXTENSIONS
OFF) are defined) so the build will fail rather than silently falling back on
older standards.

tests/data/modules/dotted_module_name/CMakeLists.txt (1)

4-5: Apply strict C++20 enforcement here as well.

Recommend adding CMAKE_CXX_STANDARD_REQUIRED ON for consistent fixture behavior across environments.

Suggested patch

 set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/data/modules/dotted_module_name/CMakeLists.txt` around lines 4 - 5, The
CMakeLists currently sets CMAKE_CXX_STANDARD and CMAKE_CXX_EXTENSIONS but
doesn't enforce the standard; update the file to explicitly require C++20 by
adding set(CMAKE_CXX_STANDARD_REQUIRED ON) so that the CMake target honors
CMAKE_CXX_STANDARD (refer to the existing set(CMAKE_CXX_STANDARD 20) and
set(CMAKE_CXX_EXTENSIONS OFF) lines) and ensures consistent fixture behavior
across environments.

tests/integration/test_modules.py (1)

53-54: Replace fixed sleep with readiness-based synchronization.

A hardcoded sleep(2.0) can be flaky and adds cumulative test latency. Prefer waiting on a concrete readiness signal/event from the client/server handshake path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` around lines 53 - 54, Replace the brittle
hardcoded await asyncio.sleep(2.0) with a readiness-based wait: remove the sleep
and await a concrete readiness signal such as a client/server handshake
completion API or event (for example await client.wait_until_ready() or await
server_ready_event.wait()), or add a small helper like wait_for_ready() that
polls/awaits the connection/handshake state (e.g., client.handshake_complete or
server.signaled_ready) so the test proceeds only when the server is truly ready.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/integration/test_modules.py (1)

400-402: ⚠️ Potential issue | 🟠 Major

Line 400 reintroduces the open/diagnostics race.

Line 400 sends didOpen before the waiter exists, so a fast publish can be missed and Line 402 will intermittently time out. Reuse client.open_and_wait() here, or arm wait_for_diagnostics() before client.open().

🔧 Suggested patch

-    leaf_uri, _ = client.open(tmp_path / "leaf.cppm")
-    event = client.wait_for_diagnostics(leaf_uri)
-    await asyncio.wait_for(event.wait(), timeout=60.0)
+    leaf_uri, _ = await client.open_and_wait(tmp_path / "leaf.cppm")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` around lines 400 - 402, The test
reintroduces an open/diagnostics race by calling client.open(...) before arming
the diagnostics waiter; change the sequence so the diagnostic waiter is created
before sending didOpen or simply replace the pair with
client.open_and_wait(tmp_path / "leaf.cppm") to atomically open and wait for
diagnostics; locate the calls to client.open, client.wait_for_diagnostics and
event.wait in tests/integration/test_modules.py and either call
client.wait_for_diagnostics(...) first and then client.open(...), or swap both
lines for a single client.open_and_wait(...) call to reliably avoid the race.

tests/integration/test_modules.py (1)

32-56: Avoid generating CMake output inside the checked-in fixtures.

Lines 37-56 create build/ and copy compile_commands.json back into the source tree under tests/data/modules/*. That leaves these fixtures stateful across runs and dirties the worktree after local test execution. The tmp_path pattern already used in test_save_recompile is safer here too—copy each fixture into a per-test temp dir before invoking CMake.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` around lines 32 - 56, The helper
_generate_cdb is creating build/ and copying compile_commands.json back into the
original fixture workspace, which dirties checked-in fixtures; instead, update
the tests to copy the fixture directory into a per-test temporary directory
(follow the tmp_path usage in test_save_recompile) and call _generate_cdb on
that temp copy so CMake runs and output stay isolated, and modify _generate_cdb
so it only writes compile_commands.json inside the provided workspace (or
returns the path to the generated compile_commands.json) without mutating the
original fixture; ensure the tests invoke the temp-copy pattern before calling
_generate_cdb and remove the shutil.copy2 back into source-tree behavior.

tests/integration/test_modules.py (1)

399-402: ⚠️ Potential issue | 🟠 Major

Arm the diagnostics wait before opening leaf.cppm.

Line 400 reintroduces the same missed-notification race: if diagnostics arrive immediately after client.open(...), the event created on Line 401 never gets set and the test can hang intermittently. Reuse open_and_wait() here, or create the event before calling open().

🔧 Suggested patch

-    leaf_uri, _ = client.open(tmp_path / "leaf.cppm")
-    event = client.wait_for_diagnostics(leaf_uri)
-    await asyncio.wait_for(event.wait(), timeout=60.0)
+    leaf_uri, _ = await client.open_and_wait(tmp_path / "leaf.cppm")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` around lines 399 - 402, The test risks a
missed-notification race because it calls client.open(...) before arming the
diagnostics wait; change the sequence to arm the diagnostics listener first
(call client.wait_for_diagnostics(...) or reuse the helper open_and_wait() that
both opens and waits) so the Event is created before invoking client.open for
"leaf.cppm" (refer to client.open, client.wait_for_diagnostics, and
open_and_wait) ensuring the event can be set even if diagnostics arrive
immediately.

tests/integration/test_modules.py (1)

59-64: Avoid the fixed post-initialize sleep.

Line 63 makes every module test pay a hard-coded 2s and can still race on slower runners. Since CliceClient already records work-done progress, prefer waiting on a concrete “CDB scan finished” signal instead of sleeping.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` around lines 59 - 64, Replace the fixed
asyncio.sleep(2.0) in the _init function with a deterministic wait for the CDB
scan work-done progress recorded by CliceClient: after calling
client.initialize(workspace) poll or await the client's work-done/progress API
(e.g., client.wait_for_work_done, client.wait_for_progress_completion, or loop
until client.work_done_records shows the "CDB scan finished" entry) and return
only once that specific "CDB scan finished" progress is observed, ensuring tests
don't rely on a hard-coded sleep.

tests/integration/test_modules.py (1)

351-353: ⚠️ Potential issue | 🟠 Major

Arm diagnostics wait before opening leaf.cppm to avoid flakiness.

On Line 351, client.open(...) happens before the waiter is created on Line 352. If diagnostics arrive immediately, the event is missed and this can intermittently timeout.

🔧 Proposed fix

-    leaf_uri, _ = client.open(tmp_path / "leaf.cppm")
-    event = client.wait_for_diagnostics(leaf_uri)
-    await asyncio.wait_for(event.wait(), timeout=60.0)
+    leaf_uri, _ = await client.open_and_wait(tmp_path / "leaf.cppm")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` around lines 351 - 353, The test creates
the diagnostics waiter after opening the file which can miss immediately
arriving diagnostics; swap the operations so you call
client.wait_for_diagnostics(...) to get the event before calling
client.open(tmp_path / "leaf.cppm") and then await the event (i.e., use the
event returned by client.wait_for_diagnostics for leaf_uri/filename prior to
invoking client.open) to ensure the test reliably observes diagnostics.

tests/conftest.py (2)

209-222: Consider caching the CDB generation to avoid redundant CMake runs.

The workspace fixture runs generate_cdb() on every test that uses the same workspace directory. For workspaces with CMakeLists.txt, this could redundantly regenerate compile_commands.json multiple times during a test session. Consider checking if build/compile_commands.json already exists before regenerating.

♻️ Proposed optimization

 `@pytest.fixture`
 def workspace(request: pytest.FixtureRequest, test_data_dir: Path) -> Path | None:
     """Resolve workspace path from `@pytest.mark.workspace`("subdir") marker.

     If the workspace contains a CMakeLists.txt, automatically runs cmake
     to generate compile_commands.json. Returns None if no marker is present.
     """
     marker = request.node.get_closest_marker("workspace")
     if marker is None:
         return None
     path = test_data_dir / marker.args[0]
-    if (path / "CMakeLists.txt").exists():
+    cdb_path = path / "build" / "compile_commands.json"
+    if (path / "CMakeLists.txt").exists() and not cdb_path.exists():
         generate_cdb(path)
     return path

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 209 - 222, The workspace fixture currently
calls generate_cdb(path) whenever a CMakeLists.txt exists, causing redundant
CMake runs; modify the logic in the workspace fixture to first check for an
existing compile_commands.json (e.g. path / "build" / "compile_commands.json")
and only call generate_cdb(path) if that file is missing or stale, keeping the
existing checks around (path / "CMakeLists.txt"). Ensure you reference the
workspace fixture and the generate_cdb function when implementing the presence
check so repeated tests sharing the same workspace skip unnecessary
regeneration.

---

186-206: Consider checking for Ninja availability alongside CMake.

generate_cdb requires both cmake and ninja to be available (since it uses -G Ninja). If Ninja is missing, CMake will fail with a confusing error. Consider validating Ninja availability upfront for a clearer error message.

💡 Proposed improvement

 def generate_cdb(workspace: Path) -> None:
     """Generate compile_commands.json using CMake with Ninja backend."""
     cmake = shutil.which("cmake")
     if cmake is None:
         raise RuntimeError("cmake executable not found in PATH")
+    ninja = shutil.which("ninja")
+    if ninja is None:
+        raise RuntimeError("ninja executable not found in PATH (required for -G Ninja)")
     subprocess.run(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 186 - 206, The generate_cdb function
currently only verifies CMake; add a check for Ninja availability by calling
shutil.which("ninja") before invoking cmake and raise a clear RuntimeError if
it's missing so users get an immediate, helpful message (update generate_cdb to
validate both cmake and ninja and include the tool names in the error text).

tests/integration/test_server.py (1)

284-293: Unused workspace parameter.

The workspace parameter is declared but never used in test_hover_on_unknown_file. While the marker is required for the fixture to work, consider if this test actually needs a workspace or if it could be restructured.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_server.py` around lines 284 - 293, The
test_hover_on_unknown_file function declares an unused workspace parameter;
either remove the workspace parameter from the test signature (keep the
`@pytest.mark.workspace`("hello_world") decorator if that marker must remain) or
rename the parameter to _workspace to signal it is intentionally unused; update
the function signature for test_hover_on_unknown_file accordingly to eliminate
the unused-parameter warning.

tests/conftest.py (1)

225-244: Add return type annotation to the client fixture.

The async generator fixture is missing a return type annotation. Adding it improves IDE support and documentation.

Suggested fix

 `@pytest.fixture`
 async def client(
     request: pytest.FixtureRequest, executable: Path, workspace: Path | None
-):
+) -> AsyncGenerator[CliceClient, None]:
     """Spawn clice server, auto-initialize if `@pytest.mark.workspace` is present."""

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 225 - 244, The async pytest fixture function
client is missing a return type annotation; update the fixture signature for
client to include an appropriate return type (e.g., ->
AsyncGenerator[CliceClient, None] or -> CliceClient depending on whether it
yields) to improve IDE typing and docs, and ensure the annotation matches how
client is used (if using yield-style fixture use AsyncGenerator with proper
typing, otherwise annotate as CliceClient); reference the client fixture and the
CliceClient class and keep the start_io and initialize calls unchanged.

tests/integration/test_modules.py (1)

7-7: Consider using relative import for consistency.

Using from tests.conftest import generate_cdb works but from conftest import generate_cdb (relative) is more conventional for imports within the same test package and matches how pytest resolves conftest.

Suggested change

-from tests.conftest import generate_cdb
+from conftest import generate_cdb

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` at line 7, Replace the absolute import in
the test with a relative import for consistency: change the import that brings
in generate_cdb (currently written as from tests.conftest import generate_cdb)
to use the package-local conftest module (from conftest import generate_cdb) so
pytest's usual conftest resolution is used and imports are consistent across
tests; update the import statement where generate_cdb is referenced in
test_modules.py accordingly.

tests/conftest.py (2)

217-222: Consider validating marker arguments for a clearer error message.

If a test uses @pytest.mark.workspace() without an argument, marker.args[0] will raise an unhelpful IndexError. A guard would provide a clearer failure message for test developers.

💡 Proposed validation

     marker = request.node.get_closest_marker("workspace")
     if marker is None:
         return None
+    if not marker.args:
+        raise ValueError("@pytest.mark.workspace requires a subdirectory argument")
     path = test_data_dir / marker.args[0]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 217 - 222, The workspace marker handling
should validate that the marker includes an argument before accessing
marker.args[0]; update the block that calls
request.node.get_closest_marker("workspace") to check marker is not None and
that len(marker.args) > 0, and if not raise a clear error (e.g.,
pytest.UsageError or ValueError) explaining that `@pytest.mark.workspace` requires
a path argument; keep the existing behavior of constructing path = test_data_dir
/ marker.args[0] and calling generate_cdb(path) when a CMakeLists.txt exists.

---

226-229: Consider adding return type annotation for the async fixture.

The fixture is well-designed—auto-initializing when @pytest.mark.workspace is present while allowing manual initialization for custom scenarios (like test_save_recompile). Adding a return type would improve IDE support.

💡 Proposed type annotation

 `@pytest.fixture`
 async def client(
     request: pytest.FixtureRequest, executable: Path, workspace: Path | None
-):
+) -> AsyncGenerator[CliceClient, None]:
     """Spawn clice server, auto-initialize if `@pytest.mark.workspace` is present."""

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 226 - 229, The async pytest fixture named
client should include an explicit return type to improve IDE/type-checker
support; add a return annotation such as -> AsyncGenerator[ClientType, None] (or
the concrete client class used in your tests) on the client fixture, import
AsyncGenerator (from typing or collections.abc depending on Python version) and
the concrete ClientType, and ensure the annotation matches how the fixture
yields/returns the client instance in client.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/conftest.py (1)

216-222: ⚠️ Potential issue | 🟡 Minor

Validate @pytest.mark.workspace arguments before indexing.

Line 219 assumes one positional string argument. @pytest.mark.workspace with no args (or wrong type) will throw unclear setup errors. Please guard args and raise pytest.UsageError with a clear message; also validate that the resolved path exists.

Suggested hardening

 marker = request.node.get_closest_marker("workspace")
 if marker is None:
     return None
-path = test_data_dir / marker.args[0]
+if len(marker.args) != 1 or not isinstance(marker.args[0], str):
+    raise pytest.UsageError(
+        '@pytest.mark.workspace requires one string argument, e.g. '
+        '@pytest.mark.workspace("modules/hello_world")'
+    )
+path = (test_data_dir / marker.args[0]).resolve()
+if not path.exists():
+    raise pytest.UsageError(f"Workspace path does not exist: {path}")
 if (path / "CMakeLists.txt").exists():
     generate_cdb(path)
 return path

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 216 - 222, The workspace marker handling
should validate marker arguments and the target path before using them: in the
block that calls request.node.get_closest_marker("workspace") check that
marker.args exists, has exactly one positional argument, and that the argument
is a string; if not, raise pytest.UsageError with a clear message about the
expected `@pytest.mark.workspace`("rel/path") usage. After resolving path =
test_data_dir / marker.args[0], verify path.exists() (and is a directory if
appropriate) and raise pytest.UsageError if the path does not exist; only then
call generate_cdb(path) if (path / "CMakeLists.txt").exists().

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cmake/toolchain.cmake (2)

7-8: Hardcoded x86_64 target limits to 64-bit Windows builds.

The --target=x86_64-pc-windows-msvc flag hardcodes the architecture. If ARM64 Windows support is needed in the future, this would need parameterization. If 64-bit only is intentional, this is fine.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmake/toolchain.cmake` around lines 7 - 8, The flags set for C and C++ are
hardcoded to "--target=x86_64-pc-windows-msvc" which forces 64-bit Windows only;
change the set(CMAKE_C_FLAGS ...) and set(CMAKE_CXX_FLAGS ...) usage to use a
configurable variable (e.g., TARGET_TRIPLE or WINDOWS_TARGET_TRIPLE) or derive
the triple from CMake variables (like CMAKE_SYSTEM_PROCESSOR or an option) and
default to the current value, then use that variable in the set() calls and
expose it as a CACHE STRING so ARM64 (or other) targets can be selected without
editing the file.

---

9-11: Linker flag inconsistency with CMakeLists.txt.

The toolchain sets -fuse-ld=lld for Windows, but CMakeLists.txt (lines 85-88) adds -fuse-ld=lld-link via target_link_options for the WIN32 case. When targeting the MSVC ABI (--target=x86_64-pc-windows-msvc), lld-link is the appropriate linker driver.

While the last -fuse-ld= flag typically wins, having both is inconsistent and potentially confusing. Consider using -fuse-ld=lld-link here for clarity, or removing these from the toolchain and relying solely on CMakeLists.txt.

Suggested change for consistency

-    set(CMAKE_EXE_LINKER_FLAGS "-fuse-ld=lld" CACHE STRING "Executable linker flags")
-    set(CMAKE_SHARED_LINKER_FLAGS "-fuse-ld=lld" CACHE STRING "Shared library linker flags")
-    set(CMAKE_MODULE_LINKER_FLAGS "-fuse-ld=lld" CACHE STRING "Module linker flags")
+    set(CMAKE_EXE_LINKER_FLAGS "-fuse-ld=lld-link" CACHE STRING "Executable linker flags")
+    set(CMAKE_SHARED_LINKER_FLAGS "-fuse-ld=lld-link" CACHE STRING "Shared library linker flags")
+    set(CMAKE_MODULE_LINKER_FLAGS "-fuse-ld=lld-link" CACHE STRING "Module linker flags")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmake/toolchain.cmake` around lines 9 - 11, Toolchain sets -fuse-ld=lld which
conflicts with CMakeLists.txt adding -fuse-ld=lld-link for WIN32; update
consistency by using lld-link for MSVC targets or remove duplication. Modify the
three linker flag settings in toolchain.cmake (the set(CMAKE_EXE_LINKER_FLAGS
...), set(CMAKE_SHARED_LINKER_FLAGS ...), set(CMAKE_MODULE_LINKER_FLAGS ...)) to
use "-fuse-ld=lld-link" when targeting the MSVC ABI
(--target=x86_64-pc-windows-msvc) or alternatively remove these sets and rely on
the WIN32-specific target_link_options in CMakeLists.txt that adds
"-fuse-ld=lld-link"; ensure the chosen approach keeps CMakeLists.txt's
target_link_options(WIN32 ... "-fuse-ld=lld-link") behavior unchanged.

tests/conftest.py (1)

190-209: Consider validating workspace path stays within test data directory.

While the workspace path originates from test markers (controlled by developers), there's no validation that the resolved path stays within test_data_dir. A marker like @pytest.mark.workspace("../../etc") would resolve outside the expected directory.

The static analysis hint (S603) about subprocess is a false positive here since shell=False is used and the path comes from test fixtures, not external input.

Optional path containment check

     path = test_data_dir / marker.args[0]
+    try:
+        path = path.resolve()
+        path.relative_to(test_data_dir.resolve())
+    except ValueError:
+        raise pytest.UsageError(
+            f"Workspace path must be within test data directory: {path}"
+        )
     if (path / "CMakeLists.txt").exists():
         generate_cdb(path)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 190 - 209, The generate_cdb function should
validate that the provided workspace path stays inside the repository test data
directory before invoking subprocess: resolve workspace (workspace_resolved =
workspace.resolve()) and resolve the test data root (e.g. test_data_dir =
Path(__file__).resolve().parent / "test_data" or the existing test_data_dir
fixture/constant), then check containment using
workspace_resolved.is_relative_to(test_data_dir.resolve()) (or compare parts for
older Python) and raise a RuntimeError if not contained; perform this check at
the start of generate_cdb (before calling cmake/subprocess) so you reject
markers like "@pytest.mark.workspace('../../etc')" early.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes